-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: custom and store self agent version + store self protocol version #800
feat: custom and store self agent version + store self protocol version #800
Conversation
doc/API.md
Outdated
@@ -92,6 +92,7 @@ Creates an instance of Libp2p. | |||
| options.modules | [`Array<object>`](./CONFIGURATION.md#modules) | libp2p [modules](./CONFIGURATION.md#modules) to use | | |||
| [options.addresses] | `{ listen: Array<string>, announce: Array<string>, noAnnounce: Array<string> }` | Addresses for transport listening and to advertise to the network | | |||
| [options.config] | `object` | libp2p modules configuration and core configuration | | |||
| [options.host] | `{agentVersion: string, protocolVersion: string}` | libp2p host options | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not inclined to allow setting protocolVersion
. agentVersion
is fine it has no impact, but any change to protocolVersion
warrants a connection closure in the spec (although we dont enforce that here). We could allow it, but we'd need to be very clear about not changing it unless you're intentionally creating an isolated network. ie: "DO NOT change unless you know what you're doing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
So, js-ipfs
has supposedly protocolVersion
assigned to 9000
. But in theory, this is not used anywhere I think. I am not sure about the reasoning behind that number, but it would need to be changed to libp2p's default value. If so, we can drop the setting of protocolVersion
unless a new use case appears
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-IPFS reports protocolVersion
as 'ipfs/0.1.0'
so it should probably change to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure why js-ipfs is trying to use that, but you are correct, it won't change anything we're actually sending over the wire. That should be corrected in js-ipfs.
Let's leave out being able to configure protocolVersion
for now. We may add it in the future, but that's a can of worms I'd rather not open right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This PR adds the ability to custom the agentVersion of libp2p, as well as to store it and the protocol version self values in the metadataBook.
I added a host property to track this custom options.
go-libp2p
also has the concept of host and it seems a good naming. When we work on the configuration improvements, we should probably think about moving other options inside the host, such as the AddressBook options.I followed the same logic of signedPeerRecords being created in the IdentifyService and added this into the Identify subsystem.
Closes #799